Skip to content

Implement Queen Attack generator#393

Merged
ErikSchierboom merged 3 commits into
exercism:masterfrom
jpreese:add-queen-attack-generator
Sep 5, 2017
Merged

Implement Queen Attack generator#393
ErikSchierboom merged 3 commits into
exercism:masterfrom
jpreese:add-queen-attack-generator

Conversation

@jpreese

@jpreese jpreese commented Sep 1, 2017

Copy link
Copy Markdown
Contributor

One thing to note is that our Example for this one isn't quite complete, so I'm going to start that now.

I'm open to suggestions on the generator for this one.. it feels like a lot, but it could just be a more complicated test to generate. Nothing felt too hacky, and most of it made sense to me.

Edit: Example updated to pass new tests.

@jpreese jpreese force-pushed the add-queen-attack-generator branch 2 times, most recently from beb5f6d to d14e62a Compare September 2, 2017 01:13
@ErikSchierboom

Copy link
Copy Markdown
Member

I'm a bit confused. I would expect the Create method to actually create a queen, not return a boolean value.

@jpreese jpreese force-pushed the add-queen-attack-generator branch from d14e62a to f7eac6f Compare September 2, 2017 12:23
@jpreese

jpreese commented Sep 2, 2017

Copy link
Copy Markdown
Contributor Author

That was my initial approach as well. I added the Create() method in the stub and had it return type Queen. Though the canonical data has it returning 0 for success, and -1 for failure, I'm not sure why it uses a number for creation, but a boolean for attacking.

Though now that I'm thinking aloud, it may be better to not take the canonical data so literally, and actually go back to Create returning Queen, but throw an exception.

@ErikSchierboom

Copy link
Copy Markdown
Member

Agreed! The canonical data can be quite weird sometimes.

@jpreese

jpreese commented Sep 2, 2017

Copy link
Copy Markdown
Contributor Author

Lets give that approach a whirl.

@jpreese

jpreese commented Sep 4, 2017

Copy link
Copy Markdown
Contributor Author

In case the github ping didn't go off for some reason, this has been changed to throw exceptions.

@ErikSchierboom ErikSchierboom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Some small nits.

public void Cannot_occupy_same_space()
public void Queen_with_a_valid_position()
{
var actual = QueenAttack.Create(2, 2);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a comment below this line to indicate that we are testing that no exception is thrown.

Comment thread generators/Exercises/QueenAttack.cs Outdated
return RenderCanAttackAssert(testMethodBody);
}

if(testMethodBody.UseVariableForTested)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add space after if

Comment thread generators/Exercises/QueenAttack.cs Outdated
{
foreach(var canonicalDataCase in canonicalData.Cases)
{
if(canonicalDataCase.Property == "create")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add space after if

Comment thread generators/Exercises/QueenAttack.cs Outdated
{
protected override void UpdateCanonicalData(CanonicalData canonicalData)
{
foreach(var canonicalDataCase in canonicalData.Cases)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add space after foreach

@jpreese

jpreese commented Sep 4, 2017

Copy link
Copy Markdown
Contributor Author

I really need to remember to update my visual studio formatting preferences.

@ErikSchierboom

Copy link
Copy Markdown
Member

I really need to remember to update my visual studio formatting preferences.

You really shouldn't have to remember, we should have the tooling do that! I've created an issue for this: #398

@jpreese

jpreese commented Sep 5, 2017

Copy link
Copy Markdown
Contributor Author

I don't think it's worth making a special template for the initial case of making a comment, so I just added it to the test description.

@ErikSchierboom

Copy link
Copy Markdown
Member

@jpreese Did you re-run the test generator after making that change? It doesn't appear in the test file :)

@ErikSchierboom ErikSchierboom merged commit fd23081 into exercism:master Sep 5, 2017
@ErikSchierboom

Copy link
Copy Markdown
Member

Thanks a lot! 🎉

@jpreese jpreese deleted the add-queen-attack-generator branch October 3, 2017 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants